ci: coverage-fanout sender + unified single Python Integration Tests check#879
ci: coverage-fanout sender + unified single Python Integration Tests check#879eric-wang-1990 wants to merge 16 commits into
Conversation
…ce PRs Wires databricks-sql-python into the multi-language coverage fan-out. When a PR merges to main and touched driver source (a file under src/), dispatch a `coverage-fanout` repository_dispatch to databricks/databricks-driver-test. Its coverage-fanout-tracker.yml then opens a tracking issue and runs the language-agnostic fan-out (a spec authored from this PR's diff, conformed across every driver). - Adds `closed` to the pull_request trigger types; the new trigger-coverage-fanout job gates on pull_request.merged == true. - Source-path filter (src/): docs/CI/test-only merges don't warrant a full fan-out. - Reuses the existing INTEGRATION_TEST App token (scoped to driver-test) + the same peter-evans/repository-dispatch pin adbc-drivers/databricks uses. - Tightens skip-integration-tests-pr's guard to exclude `closed` so it doesn't re-stamp a check on merged PRs. Co-authored-by: Isaac Signed-off-by: Eric Wang <e.wang@databricks.com>
GitHub Actions
Inactive
GitHub Actions
Inactive
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — a well-scoped CI addition that mirrors the existing dispatch pattern. One low-severity note: the new trigger-coverage-fanout job omits an explicit permissions: block that every sibling job in this file declares, so its pulls.listFiles call relies on the org's default GITHUB_TOKEN permissions. The closed-event guards and skip-integration-tests-pr exclusion are correct, and the client-payload has no injection surface (no PR-title interpolation).
eric-wang-1990
added
the
engineer-bot
GitHub Actions
Inactive
Addresses: - #3641231149 at .github/workflows/trigger-integration-tests.yml:468 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Medium
Looks good overall — the closed event is added cleanly and all existing jobs have explicit event/action guards, so none misfire on it (and skip-integration-tests-pr correctly excludes closed). One medium concern: the new trigger-coverage-fanout job has no base-branch guard, so it fires for merges into any branch, not just main as the description intends.
Addresses: - #3641618444 at .github/workflows/trigger-integration-tests.yml:459 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — the closed-event guard audit checks out: every pre-existing job correctly ignores closed (synchronize/labeled/merge_group), skip-integration-tests-pr now excludes it as intended, and the new trigger-coverage-fanout job is properly gated on merged == true + base.ref == 'main'. Payload interpolations are injection-safe and the failure-handler gate reasoning is sound. One low-severity note about duplicate dispatch on manual re-runs.
Addresses: - #3641958020 at .github/workflows/trigger-integration-tests.yml:540 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Medium
Looks solid — the closed event is correctly fenced off from every pre-existing job, and the new trigger-coverage-fanout job's gating/failure-handler reasoning is sound. One medium concern: for merged fork PRs, pull_request events don't carry secrets, so the fan-out (and its failure comment) can silently no-op.
Addresses: - #3641977677 at .github/workflows/trigger-integration-tests.yml:494 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Medium
Looks solid overall — the closed-event wiring is well-guarded (every other job correctly excludes the new event) and the failure-handler/truncation reasoning is sound. One medium concern: fork src merges aren't cleanly excluded at the job level, so they produce a red workflow run rather than the silent exclusion the header comment claims.
Addresses: - #3641995560 at .github/workflows/trigger-integration-tests.yml:476 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — a carefully-scoped CI change with thorough gating and inline rationale. I verified the whole workflow: on the new closed/merged event no existing job misfires (remove-label-on-new-commit gates on synchronize, skip-integration-tests-pr now excludes closed, trigger-tests-pr on labeled, merge-queue-python on merge_group), and the new trigger-coverage-fanout job correctly requires merged==true + base main + non-fork, serializes via concurrency, and fails safe on the source-detection path. One low-severity permissions observation noted inline.
Addresses: - #3642007200 at .github/workflows/trigger-integration-tests.yml:493 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — a focused, well-documented CI change. Verified every job guard is safe against the newly-added closed event (only the new trigger-coverage-fanout job fires on it, correctly gated on merged == true), and the dispatch payload carries no user-controlled free text so there's no injection risk. One low edge-case note on the fork-exclusion condition when a head repo has been deleted.
The "Integration tests triggered" PR comment (and two header comments) linked to python-proxy-tests.yml, which does not exist (404) — the receiver file is databricks-sql-python-proxy-tests.yml. Point to the correct workflow and note that the authoritative results are the "Python Proxy Tests / thrift" and "Python Proxy Tests / kernel" checks posted back on the PR. Co-authored-by: Isaac Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
Addresses: - #3643591969 at .github/workflows/trigger-integration-tests.yml:455 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
|
Integration tests triggered. View workflow runs. Results post back here as the "Python Proxy Tests / thrift" and "Python Proxy Tests / kernel" checks. |
Python was the last multi-backend driver posting per-mode checks (`Python Proxy Tests / thrift` + `/ kernel`). The new standalone receiver in databricks-driver-test (databricks-python-integration-tests.yml) fans out both backends internally and reports ONE aggregated check, matching go/nodejs. Collapse the five `MODES=['thrift','kernel']` two-check loops (skip / label auto-pass / dispatch-fail / merge-queue auto-pass / merge-queue-fail) into a single `Python Integration Tests` check, add `proxy_mode: replay` to both dispatch payloads for parity with the other senders, and fix the PR-comment link to the new receiver workflow. The dispatch itself was already a single `python-pr-test` (no per-mode fan-out on the sender). Follow-up (branch protection, done separately): swap the required-checks list — remove `Python Proxy Tests / thrift` and `/ kernel`, add `Python Integration Tests`. Sequenced after this + the receiver land to avoid a deadlock. Co-authored-by: Isaac Signed-off-by: eric-wang-1990 <e.wang@databricks.com>
|
Integration test approval reset. New commits were pushed to this PR. The A maintainer must re-review the changes and re-add the label to trigger tests again. Latest commit: 419cadb |
There was a problem hiding this comment.
Verdict: 1 Low
Looks good — CI-only change that cleanly collapses the per-mode Python Proxy Tests checks into a single Python Integration Tests check and adds a well-guarded post-merge coverage fan-out. Job gating (closed/merged/non-fork), the source != 'false' failure-handler logic, and the 3000-file truncation handling are all sound. One low-severity note on a change-detection divergence between the IT gate and the fan-out gate.
Addresses: - #3643898716 at .github/workflows/trigger-integration-tests.yml:484 Signed-off-by: peco-engineer-bot[bot] <peco-engineer-bot[bot]@users.noreply.github.com>
sreekanth-db
left a comment
sreekanth-db
left a comment
There was a problem hiding this comment.
Approving — reviewed with Isaac Review, no blocking issues.
Summary
Brings the databricks-sql-python CI sender fully in line with the other drivers' cross-repo integration flow. Three related changes to
.github/workflows/trigger-integration-tests.yml:Coverage fan-out on merge — on a source-affecting merge to
main, dispatchcoverage-fanouttodatabricks-driver-test, whose tracker opens a tracking issue and runs the language-agnostic fan-out. (Fork merges are intentionally excluded — no secret access; picked up by the next maintainer-branch merge.)Unified single check — the driver-test receiver (
databricks-python-integration-tests.yml, its paired PR) now fans out the thrift + kernel backends internally and reports one aggregatedPython Integration Testscheck, matching go/nodejs. This collapses the fiveMODES=['thrift','kernel']two-check loops (skip / label auto-pass / dispatch-fail / merge-queue auto-pass / merge-queue-fail) into a singlePython Integration Testscheck, and addsproxy_mode: replayto both dispatch payloads (the PR gate is replay-only, like every other sender).Comment-link fix — the "tests triggered" PR comment now links to the correct receiver workflow (
databricks-python-integration-tests.yml) and names the single check.The dispatch was already a single
python-pr-test(no per-mode fan-out on the sender side). The shared reusable workflow + weekly slow cron in driver-test are untouched — they keep their per-modePython Proxy Tests / <mode>checks for cron use.Ordering / follow-up
Python Integration Testscheck name.Python Proxy Tests / thrift+/ kernel, addPython Integration Tests. Doing it out of order would deadlock PRs, so it is sequenced last.Test Plan
Python Integration Tests; both dispatches carryproxy_mode: replay; noMODESloop remainstrigger-coverage-fanoutjob preserved (source-gated, fork-excluded, serialized per PR)workflow_dispatch -f proxy_mode=replayfans out thrift+kernel → single greenPython Integration TestscheckThis pull request and its description were written by Isaac.